Add migration support to the SSA client#480
Add migration support to the SSA client#480metlos merged 3 commits intocodeready-toolchain:masterfrom
Conversation
metlos
requested review from
MatousJobanek,
alexeykazakov,
fbm3307,
jrosental,
mfrancisc,
rajivnathan,
rsoaresd and
xcoulon
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #480 +/- ##
==========================================
- Coverage 78.95% 78.73% -0.23%
==========================================
Files 52 52
Lines 2604 2647 +43
==========================================
+ Hits 2056 2084 +28
- Misses 490 501 +11
- Partials 58 62 +4
🚀 New features to boost your workflow:
|
|
| // The user agent can be obtained from the REST config from which the client is constructed. | ||
| // | ||
| // The user agent in the REST config is usually empty, so there's no need to set it here either in that case. | ||
| NonSSAFieldOwner string |
There was a problem hiding this comment.
in your code in the previous PR, you originally had a code that was able to construct the owner name we should migrate from - couldn't we use that?
https://github.com/metlos/toolchain-common/blob/6ff87dfbb853a19475dff63b22a0e5a2f51300d9/pkg/client/ssa_client.go#L50-L62
There was a problem hiding this comment.
We technically don't need this because no code in the current codebase sets the field owner explicitly in the CRUD operations and therefore all the code uses the default field owner derived from the user agent.
There was a problem hiding this comment.
I wanted to have this option available though in case I missed something and we for example set the user agent of the kubernetes client to an explicit value somewhere in the code.
In that case, we'd need to set this to an explicit value, too.
| if len(oldFieldOwner) == 0 { | ||
| // this is how the kubernetes api server determines the default owner from the user agent | ||
| // The default user agent has the form of "name-of-binary/version information etc.". | ||
| // The owner is the first part of the UA unless explicitly specified in the request URI. | ||
| oldFieldOwner = strings.Split(rest.DefaultKubernetesUserAgent(), "/")[0] | ||
| } |
There was a problem hiding this comment.
related to my previous comment, do you think that we would need to se the owner name explicitly? Couldn't we rely on the default behavior only?
There was a problem hiding this comment.
We could, but the default behavior makes assumptions that I was not 100% sure we satisfy everywhere in the codebase, so I wanted to have an explicit option available for the case we find a place where the defaults would not be applicable.
The default behavior assumes that:
- There is no user agent set explicitly in the config of any kubernetes client.
- We don't explicitly set the field owner with any of the existing CRUD operations with the kubernetes clients.
There was a problem hiding this comment.
There is no user agent set explicitly in the config of any kubernetes client.
We don't explicitly set the field owner with any of the existing CRUD operations with the kubernetes clients.
I'm not aware of any place that would change any of these explicitly, so everything should comply.
Anyway, thanks for the explanation. It's fine to keep it there if not required to be set explicitly by default (only as a backup solution for some corner-cases).
2 participants
The migration from CSA (client-side-apply) to SSA is in the end needed (we first thought we could do without it), because the "dangling" update entries in the managed fields could prevent the automatic field removal in some future update (that could happen many operator versions later, as long as the object still survives from the times where our operators didn't use SSA for creating/updating objects in the cluster).